get_differential_vars: check Diagonal.diag, not the full matrix - #3923
Open
singhharsh1708 wants to merge 1 commit into
Open
get_differential_vars: check Diagonal.diag, not the full matrix#3923singhharsh1708 wants to merge 1 commit into
singhharsh1708 wants to merge 1 commit into
Conversation
Member
|
The GPU job on this PR gets past the This appears to confirm that this PR cleared the earlier scalar-indexing barrier, after which CI reached the unrelated QR barrier. I would keep this PR focused; its GPU job cannot become green until the CUDA fix is available in the resolved environment. |
singhharsh1708
force-pushed
the
fix-gpu-diagonal-massmatrix-scalar-index
branch
from
July 18, 2026 07:32
8281897 to
ff724e9
Compare
singhharsh1708
force-pushed
the
fix-gpu-diagonal-massmatrix-scalar-index
branch
from
July 21, 2026 14:31
ff724e9 to
59b1424
Compare
singhharsh1708
force-pushed
the
fix-gpu-diagonal-massmatrix-scalar-index
branch
from
July 30, 2026 21:25
59b1424 to
aaef682
Compare
singhharsh1708
force-pushed
the
fix-gpu-diagonal-massmatrix-scalar-index
branch
from
July 31, 2026 09:19
aaef682 to
3410aad
Compare
all(!iszero, mm) visits every (i, j) pair of the mass matrix via
scalar getindex. For a GPU-backed diagonal mass matrix
(Diagonal{Float64, <:CuVector}) that indexing is disallowed by
default, so the check throws whenever it runs on a device array,
and it is O(n^2) for a structurally O(n) question. Diagonal
guarantees off-diagonal zeros, so check mm.diag directly; the
broadcast over the diagonal vector dispatches to a device kernel,
matching what find_algebraic_vars_eqs(::Diagonal) already does.
Adds a guard test: a Diagonal wrapping a vector that errors on
scalar getindex but supports whole-array broadcast, i.e. device
array semantics on the CPU, so a regression back to the
whole-matrix walk is caught without GPU hardware in CI.
The statically resolvable Diagonal branch also fixes inference of
solve for the two Rodas4 + ShampineCollocationInit cases in the
NonlinearSolve mass matrix tests. Their form
@test_broken sol = @inferred solve(...) errors with "expression
evaluated to non-Boolean" once @inferred succeeds, so those two
lines become @test (@inferred ...; true), still erroring if
inference regresses.
Also wires up show_errors(results) in the GPU DAE test entry
point, which was defined but never called, so a real failure
prints its actual exception instead of just a status glyph.
singhharsh1708
force-pushed
the
fix-gpu-diagonal-massmatrix-scalar-index
branch
from
July 31, 2026 20:09
3410aad to
806ae08
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rework of the original patch, rebased onto current master (0be95df).
The GPU DAE lane has recovered on master without this change. After the CUDA environment rework on the self-hosted runner,
GPU: dae_tests.jlstands at 136 passed / 1 failed / 7 broken as of master run 30340807357 (Jul 28); the one remaining failure isHairer4 [jac=CSC, mass=diag_cu], a different problem. So the earlier "fixes 100% GPU DAE failure" framing no longer applies, and #3922 should be closed or retitled to track the remaining Hairer4 case. What this PR still fixes is the access pattern.get_differential_vars(lib/OrdinaryDiffEqCore/src/misc_utils.jl) checks the mass matrix withall(!iszero, mm)before its_isdiagbranch. For aDiagonalthat walks all n^2 positions, and every diagonal hit is a scalargetindexon the underlying vector, which is disallowed by default when that vector is aCuVector, and O(n^2) for a question the type answers in O(n). The fix adds aDiagonaldispatch that broadcasts overmm.diag, so the reduction is O(n) and stays on device. This is the same diagonal-only reductionfind_algebraic_vars_eqs(::Diagonal)in the same file performs, though that one goes throughdiag(M)rather than the field.CPU results are unchanged. Mixed, all-nonzero and all-zero diagonals return exactly what the old
_isdiagpath returned, including thereshapetosize(u)for matrix-valuedu, and all three are covered by new tests.The access pattern itself is now tested on CPU: a
Diagonalwrapping a vector type that errors on scalargetindexbut supports whole-array broadcast, i.e. device-array semantics without a GPU. On master's code path that construction throws insideall(!iszero, mm); with this patch it returns the correct differential-vars mask.One test adjustment was required. The
Diagonalbranch is statically resolvable, which fixes inference ofsolvefor the two Rodas4 +ShampineCollocationInitcases atlib/OrdinaryDiffEqNonlinearSolve/test/mass_matrix_tests.jl:385/387. Those lines were@test_broken sol = @inferred solve(...), a form that errors with "Expression evaluated to non-Boolean" once@inferredsucceeds, because the successful@inferredreturns the solution object. Both lines now read@test (@inferred solve(...); true): green while inference holds, an error if it regresses. I checked the replacement form against master's code and it does error there, so it is not a rubber stamp. TheRosenbrock23andRodas5lines are untouched and stay@test_broken; those two fail becausesolveitself throwsCheckInitFailureError, which this change does not affect.Also wires up
show_errors(results)in the GPU DAE test entry point; it was defined but never called, so a failing case only ever logged a status glyph with no exception text. That file runs only on the self-hosted GPU lane.Local verification on Julia 1.12.6, rebased on master:
lib/OrdinaryDiffEqCore/test/algebraic_vars_detection_tests.jl: 22 passed / 0 failed (18 on master, so 4 new assertions).lib/OrdinaryDiffEqNonlinearSolve/test/mass_matrix_tests.jl: 227 passed / 0 failed / 0 errored / 40 broken. Master baseline on the same file is 225 passed / 42 broken; the two that flip are the Rodas4 lines above.lib/OrdinaryDiffEqBDF:dae_convergence_tests.jl50/50,dae_initialization_tests.jl38 passed / 2 broken (pre-existing).No GPU hardware here, so the CUDA path is covered by the CPU device-semantics guard test rather than on-device.
The red checks on this PR are inherited from master.
format-checkfails onlib/OrdinaryDiffEqCore/src/disco.jl,lib/OrdinaryDiffEqCore/src/integrators/controllers.jlandtest/Integrators_I/disco_tests.jl, none of which this PR touches and all of which are dirty in a clean master checkout. Thelib/OrdinaryDiffEqNonlinearSolveCore lane dies inlinear_solver_tests.jlwithUndefVarError: MatrixOperatorbefore it ever reachesmass_matrix_tests.jl; the same failure is on master run 30610465511. Both need fixing separately.AI Disclosure
Claude assisted with this work.